Skip to content

Ensure Quark runs with a compatible Rizin #337

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open

Ensure Quark runs with a compatible Rizin #337

wants to merge 20 commits into from

Conversation

haeter525
Copy link
Member

@haeter525 haeter525 commented Apr 25, 2022 •
edited
Loading

Description
Fix #332.
This PR aims to -

  • determine which Rizin executable Quark should use for the Rizin-based analysis.
  • compile and install an independent Rizin if necessary.

It consists of three steps to find a Rizin executable.

  1. Check if the user specifies a path for Rizin (by option --rizin-path).
  2. Check if a compatible Rizin executable exists in the system PATH.
  3. Otherwise, install an independent Rizin in the Quark directory (~/.quark-engine) and use it for the analysis.

Note that a user can use the flag --disable-rizin-installation to disable the installation of the independent Rizin.
In this way, Quark will terminate the execution if no compatible Rizin exists.

Test Plans

  • Add seventeen tests for this logic.
  • Ensure all tests pass.

@pep8speaks
Copy link

pep8speaks commented Apr 25, 2022 •
edited
Loading

Hello @haeter525! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-09-08 09:42:13 UTC

@lgtm-com
Copy link

lgtm-com bot commented Apr 25, 2022

This pull request introduces 1 alert when merging 96dcd7c into 155e76c - view on LGTM.com

new alerts:

  • 1 for Except block handles 'BaseException'

@haeter525 haeter525 force-pushed the add_automatic_installation_for_rizin branch from 759d29f to f8e71a8 Compare May 8, 2022 13:30
@codecov-commenter
Copy link

codecov-commenter commented Sep 2, 2022 •
edited
Loading

Codecov Report

Base: 76.99% // Head: 77.61% // Increases project coverage by +0.61% 🎉

Coverage data is based on head (019b94f) compared to base (9af6e12).
Patch coverage: 84.65% of modified lines in pull request are covered.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master     #337      +/-   ##
==========================================
+ Coverage   76.99%   77.61%   +0.61%     
==========================================
  Files          60       60              
  Lines        4456     4788     +332     
==========================================
+ Hits         3431     3716     +285     
- Misses       1025     1072      +47
Flag Coverage Δ
unittests 77.61% <84.65%> (+0.61%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
quark/cli.py 0.00% <0.00%> (ø)
quark/core/parallelquark.py 0.00% <0.00%> (ø)
quark/utils/tools.py 72.56% <77.68%> (+14.42%) ⬆️
quark/report.py 97.14% <92.85%> (-2.86%) ⬇️
tests/utils/test_tools.py 98.03% <97.29%> (-1.97%) ⬇️
quark/config.py 100.00% <100.00%> (ø)
quark/core/axmlreader/__init__.py 83.53% <100.00%> (ø)
quark/core/quark.py 72.86% <100.00%> (+0.30%) ⬆️
quark/core/rzapkinfo.py 76.15% <100.00%> (+0.43%) ⬆️
quark/utils/pprint.py 100.00% <100.00%> (ø)
... and 1 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@pulorsok
Copy link
Member

pulorsok commented Sep 6, 2022

Hi @haeter525,
I think for better user experience.
We should simplify the steps for this feature.

I suggest we can adjust it to:

  1. Check if the user environment installed compatible Rizin.
  2. If not, ask the user,
    "do you want to install Rizin version x.x?" or
    "do you want to specify Rizin path?".
    And let the user input answer.

In that case, we don't need to add new click options --rizin-path and --disable-rizin-installation.
I think it is more intuitive for the user.
What do you think?

@haeter525
Copy link
Member Author

Thanks for the suggestion, @pulorsok.
I agree that inputting the answers is more intuitive than choosing the flags. I will update the PR.

@lgtm-com
Copy link

lgtm-com bot commented Sep 8, 2022

This pull request introduces 1 alert when merging 6a2042b into 871e798 - view on LGTM.com

new alerts:

  • 1 for Wrong name for an argument in a call

@haeter525 haeter525 force-pushed the add_automatic_installation_for_rizin branch from da4fdcd to 2489919 Compare September 8, 2022 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
pr-processing-state-06
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Ensure Quark runs with the compatible Rizin
4 participants
@haeter525 @pep8speaks @codecov-commenter @pulorsok